TBH I find this less readable than the original.

TBH I find this less readable than the original.

I guess there will be no consensus, but I'll point out the problems with reviewing the original:

• Long lines wrap and make it difficult to differentiate between the pattern and variances of it (not the diff between old and new, but rather looking only at new, the vertical pattern that forms when the commands are basically the same but slightly different). Shorter lines would make this easier.
• Repeated calls to kubectl for the same thing could be combined into one. With the benefit of long kubectl command lines being reduce to a single echo "$DESCRIPTIVE_NAME". But I'll admit that the abstraction could be seen as harder to read.
• Existing scripts were invalid because kuttl runs them via sh -c, without -e... so only the success/failure of the last command in a script was counted.
  • In these updated ones, there is a shebang, but I just realised this is meaningless inside sh -c... but at least we should set -euo pipefail (although I though -o pipefail was only available in bash).
• Scripts should be checked against shellcheck, ideally they should be files so CI checks them. Common issues I found were the lack of double quotes around variable expansion, or the use of double quotes inside double quotes which won't have the intended result.

Would it perhaps be better to write tests in a language other than a shell-script language? I think sh/bash (or at least its conventions) can make what should be readable code into difficult to read code.

@adwk67 I understand and I don't want to judge on any of that. Not the neccesary experience.

To me it's important to have the same style everywhere to avoid longish test interpretations and excess development work. I'm happy with both solutions, now I follow the discussion of stackabletech/superset-operator#533 where the discussion originate from, a few PR's have been done already. ( nothing we can't revert )

I'm also more than happy to facilitate a discussion or draft a decision. We need to find common ground on how to write test scripts and tests in general.

I've discussed with @NickLarsenNZ briefly the possibility to solve stuff with templating having scripts which are general enough to be used in all tests using such style of asserts. Happy to do that either.

To be more specific (and hopefully a little more helpful!), I found the original change (for airflow, I think) absolutely the right thing, as we had repeated cases. But with this one I find it harder to see what I am checking for. Wrapped lines are a pain, but in these two lines

POD=$(kubectl -n $NAMESPACE get pod -l app.kubernetes.io/component=master,app.kubernetes.io/role-group=default -o name | head -n 1 | sed -e 's#pod/##')
      kubectl -n $NAMESPACE get pod $POD -o yaml | yq '.spec.containers[0].env[] | select (.name == "TEST_VAR_FROM_MASTER").value' | grep 'MASTER'

it says to me: "identify a pod by name and rolegroup, look up the envvars and read the value for a named one". With the new version I find my eyes having to move back up the page to see what my variable-ized kubectl call was referring to.
I've no strong opinions about this and don't want to block the PR, and I certainly agree in changing aspects of the checks that were broken anyway (your last two points, @NickLarsenNZ).

To be more specific (and hopefully a little more helpful!), I found the original change (for airflow, I think) absolutely the right thing, as we had repeated cases. But with this one I find it harder to see what I am checking for. Wrapped lines are a pain, but in these two lines

POD=$(kubectl -n $NAMESPACE get pod -l app.kubernetes.io/component=master,app.kubernetes.io/role-group=default -o name | head -n 1 | sed -e 's#pod/##')
      kubectl -n $NAMESPACE get pod $POD -o yaml | yq '.spec.containers[0].env[] | select (.name == "TEST_VAR_FROM_MASTER").value' | grep 'MASTER'

it says to me: "identify a pod by name and rolegroup, look up the envvars and read the value for a named one". With the new version I find my eyes having to move back up the page to see what my variable-ized kubectl call was referring to. I've no strong opinions about this and don't want to block the PR, and I certainly agree in changing aspects of the checks that were broken anyway (your last two points, @NickLarsenNZ).

Yeah good point. The jumping back up and down (or rather, the consequence of abstracting) is a pain too.

Thinking a little more about this, from the review perspective, I want to understand what the author is intending the assertions to be, then understand if that lines up with what is written. So perhaps commentary could help here too.

The wrapping is still a pain for me (and in your example above, I couldn't read the right hand side because the github UI puts a big copy button over it (doesn't happen in review, just pointing it out here).

Here is another go at spreading across multiple lines to reduce wrapping, without abstracting commands, and adding comments.

# Get the first pod name via label lookup, stripping the pod/ prefix
POD=$(
  kubectl -n "$NAMESPACE" get pod \
  -l app.kubernetes.io/component=master,app.kubernetes.io/role-group=default \
  -o name \
  | head -n 1 \
  | sed -e 's#pod/##'
)

# Assert that the pod contains 'MASTER' in the 'TEST_VAR_FROM_MASTER' env var
kubectl -n "$NAMESPACE" get pod "$POD" -o yaml \
  | yq '.spec.containers[0].env[] | select (.name == "TEST_VAR_FROM_MASTER").value'
  | grep 'MASTER'

The comments might seem redundant, but take the following case which might show a copy/paste error that could be picked up in review:

# Assert that the pod contains 'MASTER' in the 'TEST_VAR_FROM_MASTER' env var
kubectl -n "$NAMESPACE" get pod "$POD" -o yaml \
  | yq '.spec.containers[0].env[] | select (.name == "TEST_VAR_FROM_MASTER").value'
  | grep 'SECONDARY'

"oops, it looks like we are lookup up the wrong thing, or is the comment wrong?"

Long story short, is the example above any better for you @adwk67?

That example is a big improvement, I'd say, and there may well still be cases where it makes more sense to extract specific things out into variables like your original proposal.
So I'd go with:

1. line wrapping
2. add comments
3. judicous use of variable-ized information where it aids readabilty and comprehension (subjective, I know)
   (4. look at using another language - one for the back burner, probably)

That example is a big improvement, I'd say, and there may well still be cases where it makes more sense to extract specific things out into variables like your original proposal. So I'd go with:

1. line wrapping

2. add comments

3. judicous use of variable-ized information where it aids readabilty and comprehension (subjective, I know)
   (4. look at using another language - one for the back burner, probably)

I agree with all points.

Considering I caused this whole dilemma to surface, I'll check in with @Maleware and see where I can help to achieve those points in this PR, the come back to see if that resolves the readability issue your raised above.

Small update while I'm editing things, I can definitely see how the existing style (before this PR) is ideal. It is just problematic during in the Github UI with the word wrapping during review. (could Github please add overflow to the split-pane reviews instead of wrapping?)

So fixing it for review makes it worse for editing, and vice-versa.

I will push up my edits anyway just so we can take a look (we can always revert them).

I have added a number of commits, each concerning a single thing. I have also added additional commit comments to give reasoning.

As I mentioned earlier, if some/all of these commits are not tolerable (please consider both writing/maintenance as well as review perspectives), I can revert them.

For what it is worth, here is the problem I had with the original change (stackabletech/superset-operator#533) that started this set of changes. It wasn't the changed line specifically, but rather the continued pattern of very long lines (which I admit are fine in my text editor) where the wrapping makes it really hard for me to review:

I have added a number of commits, each concerning a single thing. I have also added additional commit comments to give reasoning.

I would agree with all of these. And thank you for taking the time to give this considerable thought! (and to @Maleware for going through all the tests to make things as consistent as possible).

Also to note: in these commits there are still duplicate calls to kubectl in the assertions, but since it is not a hot-path (given they are tests), I think the readability that @adwk67 described in an earlier comment is preferred over hiding it behind a variable (which is what I had suggested to do in stackabletech/superset-operator#533).

@Maleware shall I revert the long-opts commit (fc6fd79)? It was more of an example showing the use of long-opts to make them clear - but they shouldn't be needed on common commands.

@NickLarsenNZ Yeah I think it's morre readable without long variants.

LGTM for me too!

Thanks for your work folks :)